Merged
Conversation
Since the `Id` field isn't reflected in `Topic.Attributes`, it requires special handling.
If a property name corresponds to an attribute named `{Property}Id`, that value points to a topic in the supplied `ITopicRepository`, and that topic maps to a type assignable to the property, then the property is populated with that mapped instance.
Used to separate `private` reflection-oriented classes from `public` Topic-specific classes.
Instead of maintaining a `PropertyInfoCollection`, abstracted it out into a generic `MemberInfoCollection{T}`, and migrated dependencies to use this. Additionally included a non-generic `MemberInfoCollection` for convenience.
Converted `GetProperty()` to `GetMember()` and generic `GetMember<T>()`; converted `HasProperty()` to `HasMember()` and `HasMember<T>()`.
This is more explicit, and will provide better consistency with future members (such as the proposed `SetMethodValue()` and `GetMethodValue()`).
…e()`,
These provide counterparts to `HasSettableProperty()` and `SetPropertyValue()` for use with method-setters. Assumes form of `void Set{Property}({type} {value})` where `{type}` is one of the `SettableTypes`.
Previously, `TopicMappingService` made a reflection call to `GetRuntimeMethod()` each time it came across a `HasSettableProperty()`. This has been updated to use the new `GetMethodValue()`, which reduces the amount of code (slightly) and, more importantly, improves performance by caching reflection data for methods.
The internal cache is intended to help avoid scenarios where the same topic is referenced from multiple places within a mapped object graph. This will avoid wasting resources (by mapping the same object)—and, potentially, avoid infinite loops in some (admittedly outside) scenarios.
It is fully expected that `GetMethodValue()` might return an `int` or `bool` or `DateTime`. With `as string` those would be treated as `null`. Instead, converted to `ToString()`. Negatively, this means reference objects might be converted to their default `ToString()` behavior—but that should be an outside case where there is a `Get{Property}()` method that the developer does not intend to call and which returns an incompatible content type.
This is more accurate than `SetMemberTest()` since it explicitly tests setting a _method_.
This determines whether the type of a property or method is in the `SettableTypes` collection—or, alternatively, an optionally supplied target type.
These complement the existing `HasSettableProperty()` and `SetPropertyValue()`. While these are simple methods, they round out the interface and provide a more consistent read/write experience.
Introduced the validation routine, `HasGettableMethod()`, and added it to `GetMethodValue()` alongside the new (optional) `targetType` parameter.
Cases test both typed version (with a `DateTime` target type) and the default version (which assumes any of the supported types).
Previousy, if there was a property on topic named the same value as a property on the target DTO, the mapping library wouldn't bind them. This is because it was primarily looking for e.g. `Attribute.GetValue()` mappings. This new addition provides support for 1:1 property matching—at least assuming `String`, `Int`, `DateTime`, or `Bool`. This also eliminates the need for the `Id` fix (since `Id` doesn't have an attribute, but can now be mapped using the generic `GetPropertyValue()` technique).
This is more intuitive since the `[Recurse()]` attribute does not instruct the `ITopicMappingService` to be _truly_ recursive; it only works that way if the attribute is _chained_. The semantics of `[Follow()]` make this more clear. While I was at it, I also refactored the `Relationships` property to use an automatic implementation.
The `[Flatten]` attribute allows a collection to include all child objects. Those will then be filtered by target type and any other attributes, such as `[FilterByAttribute]`. If the collection enforces uniqueness (e.g., for a `KeyedCollection` or `Dictionary`) then duplicates will be removed.
This reduces the amount of code dedicated to assessing attributes by centralizing the repetitive aspects of the code.
This ends up actually _adding_ lines of code, _but_ it makes the `SetProperty()` method a bit easier to read, _and_ it introduces the (potentially) helpful `RelationshipMap` class, which offers a mapping between corresponding `RelationshipType` and `Relationships` enum values.
Removed the class name prefix (since it's listed in the class name, and tests can be grouped by class) and removed the "Test" suffix (since these are all test methods in test classes, this should be implicit). We may revisit these again in the future, but for now they're more succinct and easier to read in Visual Studio's Test Explorer.
Since we removed the legacy `ContentType` classes, the database(s) need to be updated to use `ContentType=ContentTypeDescriptor` instead of `ContentType=ContentType`. This wasn't accounted for in the `GetContentTypeDescriptors()` method.
Added check for cases where `sourceTopic` may be null (e.g., for legacy pages not associated with a Topic).
This wasn't actually adding any value, and was causing a conflict with reflection (since it exists on both `AttributeDescriptor` and `Attribute`). Removed.
Under expected circumstances, a duplicate `MemberInfo` key should not exist. This will happen, however, if the `new` keyword is used on a member of the _current_ class (as opposed to a _derived_ class). In that case, an error will occur. This should not happen; if it does, the developer is warned about the duplicate key with a specific error message.
By overriding `InsertItem()` we ensure that the same `ArgumentException` error is thrown _anytime_ an item is added using e.g. `Add()`, `Insert()`, &c. without needing to add guard conditions to each instance, or worry about members being added independent of construction. The latter isn't an expected use case, but even without that, it prevents the need to duplicate the guard condition for both constructors.
Updated XmlDoc documentation for `InsertItem` to correctly describe the method, parameters, exceptions, and purpose (via remarks). The previous version inadvertently included the summary from a different method.
Whenever a `CachedTopicRepository` is called, it checked to see if the `_cache` was initialized. This should not be necessary. If a `CachedTopicRepository` is required, then it should immediately load its cache so it's ready for the first request. In most scenarios, this won't change the load time (as the `Load()` method will be called shortly after establishing the cache), but it does simplify the internal logic.
Generally, `GetContentTypeDescriptors()` should be able to operate independent of any underlying implementation; it is a convenience method to consolidate data loaded by `Load()`. As such, its definition can be centralized via the `TopicRepositoryBase`. As part of this, the part of `Save()` that interacts with `GetContentTypeDescriptors()` was also able to be centralized as part of `TopicRepositoryBase`.
Ensured that `ITopicRepositoryTest` is evaluating the `GetContentTypeDescriptors()` method. This is important since this method has some built-in logic. As part of this, also ensured that `FakeTopicRepository`'s `Load()` and `GetContentTypeDescriptors()` methods were implemented enough to satisfy the test requirements. This included registering all content types in the `FakeTopicRepository` to ensure that internally-created `ContentTypeDescriptor`s had been created (e.g., `Lookup`, `List`, &c.) as well as renaming `ContentType` to `ContentTypeDescriptor`.
`IsHidden` was removed from `AttributeDescriptor`. Needed to fix XmlDoc reference to point to `Topic.IsHidden` instead.
All methods in `TopicRepositoryBase` should allow optional overriding in concrete implementations, if necessary—even though `Rollback` may not generally need it, due to its nature. Fixed formatting for `args` while I was at it (since it's not near other variable declarations, the alignment isn't necessary or consistent).
There's no need to parse integer values for e.g. `TopicId` or `ParentId`; this can now be done using `GetInteger()`. This has the added benefit of allowing more concrete validation of e.g. `ParentId` to ensure it is actually an integer (instead of simply not null).
This is required by the `_BreadCrumbs.cshtml` page's `GetBreadCrumbs()` method; without this, it throws an exception, causing error pages not to render.
Not only is `GetPath()` stateless—and, thus, will be faster if it is static—but it was actually being _called_ as a static method _already_ by `FilePath.ascx` (the only known consumer of it). It's unclear why this changed, or how it ever worked. Regardless, this fixes the issue.
Previously, we were catching `Exception`, as per Microsoft's guidance. This was due to the fact that there are a number of other exceptions that can be raised during the processing of a `SqlConnection` and, especially, a `SqlDataReader`. Most of those, however, are not _expected_ exceptions, and thus should not be handled (or even wrapped in a generic exception). In addition, moved `Exception` messages to use string interpolation for cleaner code.
Previously, the `SqlTopicRepository` was throwing generic `Exception`s. This is not a best practice. It is appropriate, however, for implementations of `ITopicRepository` to throw a generic, database agnostic exception (as opposed to e.g. `SqlException`) since it should not be tied to any specific persistence technology. The `TopicRepositoryException` allows it to accomplish this.
When a `KeyedDictionary` finds a duplicate key, it provides a generic `ArgumentException` announcing this. It doesn't provide any context, however, such as the key that is being duplicated. This makes debugging difficult. To mitigate this, I've added explicit `InsertItem` overrides that detect duplicate keys and, if they exist, provide an `ArgumentException` with an explicit error message containing not only the key value, but also any other relevant context (such as any associated `Topic` or `T` type, as appropriate).
Replaced string concactenation with string interpolation.
While not strictly necessary for our needs, this is generally expected for exceptions, and recommended by Visual Studio's Code Analysis.
In preparation for pull request, increment minor version.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The primary purpose of this update is to make some improvements to the
TopicMappingServicein terms of new features, refactoring based on the initial implementation, and performance. In addition, used this as an opportunity to make some broader cleanup to the project overall, and especially to classes related to theTopicMappingService.Breaking changes
[Recurse()] to[Follow()]TypeCollection'sSetProperty()toSetPropertyValue()for consistencytypeparameter from[Relationship()]'s constructor (now a named property)ITypeLookupServicedependency onTopicMappingServiceITopicMappingServiceto useasyncmethodsTopicController, andLayoutControllerto useasyncactionsTitle,UniqueKeytoITopicViewModelinterfaceITopicViewModel…CoretoViewModels\ITopicViewModel…AttributeandContentTypecontent typesAttributeDescriptorandContentTypeDescriptorNew Features
TopicMappingService[Flatten]attribute for consolidating all children into one collectionId)Parent,DerivedTopic){Property}Idand uses theITopicRepositoryto identify a matchPropertyConfigurationclass to centralize attribute retrievalITypeLookupServiceITypeLookupServicefor finding most-derivedTypereferencesStaticTypeLocatorService(for hard-coded class names):DefaultTopicLookupService(forTopicextensions, likeContentTypeDescriptor)ViewModelTopicLookupService(for view models, such asPageTopicViewModel)FakeViewModelLookupService(for test cases, including view models for specific tests)DynamicTypeLookupService(for dynamic lookup via reflection)DynamicTopicLookupService(for looking upTopicextensions)DynamicViewModelLookupService(for looking up*TopicViewModelimplementations)TypeCollectionandMemberInfoCollectionPropertyInfoCollectiontoMemberInfoCollectionto support caching e.g.MethodInfoHas(S|G)ettableMethod(),(S|G)etMethodValue()toTypeCollectionOther
FindAll()andFindFirst()extensions toTopicthat accepts aFunc<>delegateCachedLayoutController<T>to provide internal caching of mapped navigationsImprovements
Implementing New Features
FindAll()on existingFindAllByAttribute()extension methodFindFirst()onCachedTopicRepository's existingGetTopic()methodITypeLookupServicetoTopicFactoryandTopicMappingServiceAsyncControllerandasyncactionsAttributeValueCollection.GetInteger()where appropriateOrganizational Improvements
Ignia.Topics.Reflectionnamespace for reflection-oriented codeTopicMappingServiceparametersArchitectural Improvements
NavigationTopicViewModelindependent ofPageTopicViewModelTopicRepositoryExceptionforITopicRepository, in place of previousExceptionAttributeconstructors, preferring named attributesTopicMappingService'sSetProperty()intoSetScalarValue(),SetCollectionValue(),SetTopicReference(), &c.SetCollectionValue()intoGetSourceCollection()andPopulateTargetCollection()SqlTopicRepository'sGetContentTypeDescriptors()toTopicRepositoryBasefor reuseCode Improvements
usingstatementsreadonlywhere practical(Tuple)shorthand forTopicMappingServiceand elsewhere<returns />,<exception />, &c.ConcurrentDictionaryforstaticcaches andsingletondependenciesKeyedCollectionsInsertItem()to provide more intuitive exceptionsGetRelationship()helper function toTopicMappingServiceRelationshipMapclass to aid in mappingRelationshipsandRelationshipTypestaticfor stateless methodsSqlTopicRepositoryto catch specificSqlExceptioninstead ofExceptionincatchblock$"string {interpolation}"where possibleHousecleaning
AssemblyInfowith correctTitle,Description, andVersion